Use managed HTTP stack #1618

ethomson · 2018-10-08T10:24:22Z

In an attempt to reduce our native dependencies, we can use the managed HTTP stack for connecting to HTTP remotes. This is somewhat working but lacking a number of pieces of functionality:

However, for users who do not do any networking, this may be worth evaluation.

ethomson · 2018-10-08T11:20:00Z

We could create a nuget package for this, so that @tmat can try it out. eg, LibGit2Sharp.LocalOnly or something like that. Not sure if we should disable this experimental HTTP(S) transport for that to truly ensure that it's local-only or not.

Aimeast · 2018-10-14T14:48:06Z

May I ask this is about server side smart transport?

ethomson · 2018-10-14T14:49:35Z

May I ask this is about server side smart transport?

No, libgit2 has no support for that. This is intended to allow us to use the .NET HTTP stack instead of using libgit2's networking stack. That means that we don't need to take a dependency on openssl or curl on non-Windows platforms, which has become incredibly problematic.

ethomson · 2018-10-14T17:37:48Z

Note to self: the certificate validation callback mechanism in libgit2 assumes that there is always a certificate callback and so things get quite crashy when there isn't. We'll need to support this when it's unset.

bording · 2018-11-10T15:40:22Z

LibGit2Sharp/LibGit2Sharp.csproj

@@ -1,7 +1,7 @@
 <Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
-    <TargetFrameworks>netstandard2.0;net461</TargetFrameworks>
+    <TargetFrameworks>netcoreapp2.0;net472</TargetFrameworks>


While it's ok to change the test projects to net472 if you really want to, these can't be changed like this. They need to stay as netstandard2.0;net461.

We probably shouldn't change the test projects either, though.

Why 4.6.1 and not 4.7.2? (https://twitter.com/terrajobst/status/1031999730320986112)

Right, and that's why I'm saying we need a net461 target. .NET Standard 2.0 works with net461 even if its "broken", so you need to provide a net461 target to ensure that gets chosen over the netstandard2.0 target.

See https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/cross-platform-targeting for more on that.

Libraries shouldn't target netcoreapp2.0, so that leads us back to netstandard2.0;net461 being the correct targets here.

bording · 2018-11-10T15:45:53Z

LibGit2Sharp/Core/ManagedHttpSmartSubtransport.cs

+                    var httpClientHandler = CreateClientHandler();
+                    httpClientHandler.Credentials = credentials;
+
+                    using (var httpClient = new HttpClient(httpClientHandler))


I assume you'll be changing to use the httpClient field instead? Even though the HttpClient class implements IDisposable, you're not supposed to create one and dispose it per request.

Unclear: the credentials belong to the handler and it appears that credentials can't be changed after a request has been issued. It's not clear if using a CredentialCache will be sufficient to satisfy that and support default credentials both. Possible that I could implement my own ICredentials mechanism as a last resort.

bording · 2018-11-10T15:49:17Z

LibGit2Sharp/Core/ManagedHttpSmartSubtransport.cs

+                            request.Content.Headers.Add("Content-Type", ContentType);
+                        }
+
+                        var response = httpClient.SendAsync(request).Result;


Blocking on an async call like that isn't a great idea, but I guess this will have been called from libgit2? When that happens, what thread will it be on?

Wouldn't it be more correct to do httpClient.SendAsync(request).ConfigureAwait(false).GetAwaiter().GetResult()? Accessing .Result directly in a library can cause deadlocks, no?

.GetAwaiter().GetResult() causes the same deadlocks as .Result.

(also, the comment you're responding to is a year and a half old and marked as "Outdated")

Ouch, sorry.

ethomson · 2018-11-10T23:00:56Z

Okay, with 09dce90 this is mostly working (occasional errors with "early EOF" that I don't yet understand). Moving the core library back to netstandard2.0;net461 is a hard failure. 😵

ethomson · 2020-11-23T18:38:47Z

bording · 2020-11-23T19:13:36Z

@ethomson I was going to try and push up a new preview release, but I'm getting some errors on Azure Pipelines. It's asking me to login and then saying I don't have permission, and getting 500 errors on other links. Any ideas?

ethomson · 2020-11-23T19:17:28Z

🤔 Interesting question. I can view the artifacts - is it possible that this is a thing that you need to be logged in to Azure Pipelines to download? I didn't remember that being the case, but I've also lost all the familiarity with Azure Pipelines that I once had.

I can invite you to the project though, what email address should I use? (Feel free to email it to me direct if that's easier.)

bording · 2020-11-23T19:51:03Z

I feel like I used to be able to access everything in Azure Pipelines, so maybe something changed?

I can invite you to the project though, what email address should I use? (Feel free to email it to me direct if that's easier.)

I'll send you a DM in the libgit2 slack.

bording · 2020-11-23T20:26:08Z

These changes have been published in 0.27.0-preview-0096. Give it a try and let me know if anything explodes.

AArnott · 2020-11-23T21:53:21Z

With pleasure!

AArnott · 2020-11-24T02:17:16Z

@bording A great many of my tests are failing with this exception. Is it expected that SetConfigSearchPaths become callable-once-only?

Test Name:	BuildIntegrationTests.AssemblyInfo_IncrementalBuild
Test FullName:	NerdBank.GitVersioning.Tests (netcoreapp3.1).BuildIntegrationTests.BuildIntegrationTests.AssemblyInfo_IncrementalBuild
Test Source:	D:\git\Nerdbank.GitVersioning\src\NerdBank.GitVersioning.Tests\BuildIntegrationTests.cs : line 882
Test Outcome:	Failed
Test Duration:	0:00:00

Test Name:	BuildIntegrationTests.AssemblyInfo_IncrementalBuild
Test Outcome:	Failed
Result StackTrace:	
at LibGit2Sharp.Core.NativeMethods.git_libgit2_opts(Int32 option, UInt32 level, String path)
   at LibGit2Sharp.Core.Proxy.git_libgit2_opts_set_search_path(ConfigurationLevel level, String path)
   at LibGit2Sharp.GlobalSettings.SetConfigSearchPaths(ConfigurationLevel level, String[] paths)
   at Nerdbank.GitVersioning.GitExtensions.OpenGitRepo(String pathUnderGitRepo, Boolean useDefaultConfigSearchPaths) in D:\git\Nerdbank.GitVersioning\src\NerdBank.GitVersioning\GitExtensions.cs:line 403
   at Nerdbank.GitVersioning.VersionFile.SetVersion(String projectDirectory, VersionOptions version, Boolean includeSchemaProperty) in D:\git\Nerdbank.GitVersioning\src\NerdBank.GitVersioning\VersionFile.cs:line 316
   at Nerdbank.GitVersioning.VersionFile.SetVersion(String projectDirectory, VersionOptions version) in D:\git\Nerdbank.GitVersioning\src\NerdBank.GitVersioning\VersionFile.cs:line 278
   at RepoTestBase.WriteVersionFile(VersionOptions versionData, String relativeDirectory) in D:\git\Nerdbank.GitVersioning\src\NerdBank.GitVersioning.Tests\RepoTestBase.cs:line 135
   at RepoTestBase.WriteVersionFile(String version, String prerelease, String relativeDirectory) in D:\git\Nerdbank.GitVersioning\src\NerdBank.GitVersioning.Tests\RepoTestBase.cs:line 123
   at BuildIntegrationTests.AssemblyInfo_IncrementalBuild() in D:\git\Nerdbank.GitVersioning\src\NerdBank.GitVersioning.Tests\BuildIntegrationTests.cs:line 884
--- End of stack trace from previous location where exception was thrown ---
----- Inner Stack Trace -----
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
   at LibGit2Sharp.Core.NativeMethods.TryUseNativeLibrary()
   at LibGit2Sharp.Core.NativeMethods..cctor()
----- Inner Stack Trace -----
   at System.Runtime.InteropServices.NativeLibrary.SetDllImportResolver(Assembly assembly, DllImportResolver resolver)
Result Message:	
System.TypeInitializationException : The type initializer for 'LibGit2Sharp.Core.NativeMethods' threw an exception.
---- System.Reflection.TargetInvocationException : Exception has been thrown by the target of an invocation.
-------- System.InvalidOperationException : A resolver is already set for the assembly.
Result StandardOutput:	Random seed: 1846902623

bording · 2020-11-24T06:12:52Z

I'm not aware of anything that this PR would have done that would have any impact on that.

ethomson · 2020-11-24T14:52:39Z

There shouldn't be a limit on the number of times that you can call git_libgit2_opts(GIT_OPT_SET_SEARCH_PATH, ....

But I don't understand what that has to do with:

-------- System.InvalidOperationException : A resolver is already set for the assembly.

AArnott · 2020-11-24T15:36:04Z

I don't think this PR is related either. Must be some other change. I'll open an issue.

AArnott · 2020-11-24T16:04:30Z

Next issue: the tests pass on Windows but fail on Linux. This is because I have a RuntimeIdMap.cs file that maps various distros to specific paths in the native libgit2 runtimes folder, and all those folders were replaced with a much smaller set:

linux-arm linux-arm64 linux-musl-x64 linux-x64 osx win-x64 win-x86

That's probably exactly what you were expecting, and presumably made possible because of the drop of the native HTTPS stack.
But before I go guessing, what would you migration suggestion be? Should I remove the RuntimeIdMap entirely? Change it?

AArnott · 2020-11-24T16:23:58Z

Happily, ripping the RuntimeIdMap out entirely and all its callers made everything Just Work. Love it! 💗

bording · 2020-11-24T16:52:20Z

Happily, ripping the RuntimeIdMap out entirely and all its callers made everything Just Work. Love it! 💗

Great! That was what I was hoping would work. By only having the base RIDs, all distros should just be able to use those without any special mapping.

ethomson · 2020-11-24T18:16:50Z

Thanks @bording and @AArnott for getting this across the line. I think that this is a really fantastic improvement.

AArnott · 2020-11-24T19:27:38Z

Does this mean it should work on Linux on mono as well?

bording · 2020-11-24T20:10:03Z

Does this mean it should work on Linux on mono as well?

The story is much improved there as well, though not perfect. The LibGit2Sharp.dll.config in the native binaries package will currently cause all Linux distros to try and use the linux-x64 binary, so if the distro needs the musl binary, that would still be broken. Trying to run on an ARM processor would have the same problem.

I think I can add more dllmap entries to the file to fix the ARM scenario, but I don't think there's a way to fix the musl scenario.

AArnott · 2020-11-24T20:32:12Z

For nb.gv, I think what we have is good enough. St least until users complain about arm or musl not working.

bording · 2020-11-24T20:56:44Z

The main thing that actually needs to be validated is whether or not the set of linux binaries in the package actually work on all the various distros that .NET Core works on. I'm reasonable sure that they will, but I haven't had a way to test that myself.

AArnott · 2020-11-24T21:57:14Z

but I haven't had a way to test that myself.

NB.GV uses Azure Pipelines to test a variety of linux distros. Basically any that we do something to make work, we add a regression test for. You can see it here by expanding "functional testing" here: https://dev.azure.com/andrewarnott/OSS/_build/results?buildId=3979&view=results

bording · 2020-11-24T22:03:01Z

Yeah, adding some tests with different distros via docker images would help, but there are some distros that don't have images available (RHEL, SLES).

It's something I'll have to look into eventually.

AArnott · 2020-11-25T00:37:22Z

Ya, Pop!_OS doesn't seem to have a docker image anywhere, and that has come up a few times as one that folks want NB.GV to work in. I had to create a VM and test manually.

Integrate new implementations from libgit2/libgit2sharp#1618 With the previous version, in deployments on Docker found the 'load from git' function in the Elm Editor did not work. The backend returned this error with the HTTP response: ``` Exception in volatile host: System.AggregateException: One or more errors occurred. (The type initializer for 'LibGit2Sharp.Core.NativeMethods' threw an exception.) ---> System.TypeInitializationException: The type initializer for 'LibGit2Sharp.Core.NativeMethods' threw an exception. ---> System.DllNotFoundException: Unable to load shared library 'git2-106a5f2' or one of its dependencies. In order to help diagnose loading problems, consider setting the LD_DEBUG environment variable: libgit2-106a5f2: cannot open shared object file: No such file or directory at LibGit2Sharp.Core.NativeMethods.git_libgit2_init() at LibGit2Sharp.Core.NativeMethods.InitializeNativeLibrary() at LibGit2Sharp.Core.NativeMethods..cctor() --- End of inner exception stack trace --- at LibGit2Sharp.Core.NativeMethods.git_clone(git_repository*& repo, String origin_url, FilePath workdir_path, GitCloneOptions& opts) at LibGit2Sharp.Core.Proxy.git_clone(String url, String workdir, GitCloneOptions& opts) at LibGit2Sharp.Repository.Clone(String sourceUrl, String workdirPath, CloneOptions options) at Kalmit.LoadFromGithub.LoadFromUrl(String sourceUrl) in /app/PersistentProcess/PersistentProcess.Common/LoadFromGithub.cs:line 109 at Kalmit.LoadFromPath.LoadTreeFromPath(String path) in /app/PersistentProcess/PersistentProcess.Common/LoadFromPath.cs:line 11 [...] ``` See the discussion of the problem at libgit2/libgit2sharp#1798 Another workaround found in the discussion was switching to the bionic flavor of the .net docker image.

Integrate new implementations from libgit2/libgit2sharp#1618 With the previous version, in deployments on Docker found the 'load from git' function in the Elm Editor did not work. The backend returned this error with the HTTP response: ``` Exception in volatile host: System.AggregateException: One or more errors occurred. (The type initializer for 'LibGit2Sharp.Core.NativeMethods' threw an exception.) ---> System.TypeInitializationException: The type initializer for 'LibGit2Sharp.Core.NativeMethods' threw an exception. ---> System.DllNotFoundException: Unable to load shared library 'git2-106a5f2' or one of its dependencies. In order to help diagnose loading problems, consider setting the LD_DEBUG environment variable: libgit2-106a5f2: cannot open shared object file: No such file or directory at LibGit2Sharp.Core.NativeMethods.git_libgit2_init() at LibGit2Sharp.Core.NativeMethods.InitializeNativeLibrary() at LibGit2Sharp.Core.NativeMethods..cctor() --- End of inner exception stack trace --- at LibGit2Sharp.Core.NativeMethods.git_clone(git_repository*& repo, String origin_url, FilePath workdir_path, GitCloneOptions& opts) at LibGit2Sharp.Core.Proxy.git_clone(String url, String workdir, GitCloneOptions& opts) at LibGit2Sharp.Repository.Clone(String sourceUrl, String workdirPath, CloneOptions options) at Kalmit.LoadFromGithub.LoadFromUrl(String sourceUrl) in /app/PersistentProcess/PersistentProcess.Common/LoadFromGithub.cs:line 109 at Kalmit.LoadFromPath.LoadTreeFromPath(String path) in /app/PersistentProcess/PersistentProcess.Common/LoadFromPath.cs:line 11 [...] ``` See the discussion of the problem at libgit2/libgit2sharp#1798 and libgit2/libgit2sharp#1747 Another workaround found in the discussion was switching to the bionic flavor of the .net docker image.

bording mentioned this pull request Oct 8, 2018

Cannot build on Ubuntu 18.04 TLS dotnet/roslyn#29289

Closed

ethomson force-pushed the ethomson/notls branch from e06837a to 3101db8 Compare October 14, 2018 17:38

This was referenced Oct 18, 2018

Unhandled exception on latest Ubuntu 18.04.1 because of upgrade to libcurl4 terrajobst/git-istage#13

Closed

Add Ubuntu 18.04 lib libgit2/libgit2sharp.nativebinaries#74

Merged

gep13 mentioned this pull request Oct 21, 2018

Reduce native binary dependencies GitTools/GitVersion#1511

Closed

tmat mentioned this pull request Oct 25, 2018

WIP: Experimental no-TLS version libgit2/libgit2sharp.nativebinaries#77

Closed

ethomson mentioned this pull request Nov 1, 2018

linux-x64 libcurl.so.4 ABI incompatibility #1626

Closed

This was referenced Nov 3, 2018

GetBuildVersion task fails for full Framework builds dotnet/Nerdbank.GitVersioning#228

Closed

Update LibGit2Sharp.NativeBinaries to 1.0.245 #1636

Merged

ethomson force-pushed the ethomson/notls branch 4 times, most recently from 924f342 to 8e5c177 Compare November 7, 2018 00:06

ethomson mentioned this pull request Nov 7, 2018

Update NativeBinaries to libgit2 0.27.7 #1639

Merged

ethomson force-pushed the ethomson/notls branch 2 times, most recently from 420908c to ea06a88 Compare November 10, 2018 14:43

bording reviewed Nov 10, 2018

View reviewed changes

ethomson force-pushed the ethomson/notls branch 8 times, most recently from 2513a40 to 09dce90 Compare November 10, 2018 22:44

bording deleted the ethomson/notls branch November 23, 2020 18:33

bording changed the title ~~WIP: Use managed HTTP stack~~ Use managed HTTP stack Nov 23, 2020

AArnott mentioned this pull request Nov 24, 2020

System.InvalidOperationException : A resolver is already set for the assembly. #1855

Closed

This was referenced Dec 11, 2020

Nuget package not working on Ubuntu 19.04 libgit2/libgit2sharp.nativebinaries#98

Closed

Support Ubuntu 20.04 (Focal) #1747

Closed

pascalberger mentioned this pull request Mar 12, 2021

Add support for Ubuntu 20 cake-contrib/Cake_Git#140

Open

bording mentioned this pull request Sep 16, 2021

Update to libgit2 v1.2.0 #1907

Merged

Use managed HTTP stack #1618

Use managed HTTP stack #1618

Conversation

ethomson commented Oct 8, 2018 • edited by bording Loading

ethomson commented Oct 8, 2018

Aimeast commented Oct 14, 2018

ethomson commented Oct 14, 2018

ethomson commented Oct 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethomson commented Nov 10, 2018

ethomson commented Nov 23, 2020

bording commented Nov 23, 2020

ethomson commented Nov 23, 2020

bording commented Nov 23, 2020

bording commented Nov 23, 2020

AArnott commented Nov 23, 2020

AArnott commented Nov 24, 2020

bording commented Nov 24, 2020

ethomson commented Nov 24, 2020

AArnott commented Nov 24, 2020

AArnott commented Nov 24, 2020

AArnott commented Nov 24, 2020

bording commented Nov 24, 2020

ethomson commented Nov 24, 2020

AArnott commented Nov 24, 2020

bording commented Nov 24, 2020

AArnott commented Nov 24, 2020

bording commented Nov 24, 2020

AArnott commented Nov 24, 2020

bording commented Nov 24, 2020

AArnott commented Nov 25, 2020

ethomson commented Oct 8, 2018 •

edited by bording

Loading